Skip to content

feat(orchestrator): introduce new orchestrator#2829

Open
juliusmarminge wants to merge 137 commits into
mainfrom
t3code/codex-turn-mapping
Open

feat(orchestrator): introduce new orchestrator#2829
juliusmarminge wants to merge 137 commits into
mainfrom
t3code/codex-turn-mapping

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented May 27, 2026

Copy link
Copy Markdown
Member

Summary

  • wire orchestration V2 provider adapter registry/factory flow for Codex and Claude provider instances
  • add Claude replay/query primitives, native fork/rollback fixtures, subagent fixture coverage, and provider replay harness updates
  • update debugger model/provider picker and improve user-facing orchestration errors

Validation

  • bun fmt
  • bun lint
  • bun typecheck
  • bun run test -- src/orchestration-v2/testkit/OrchestratorReplayFixtures.integration.test.ts -t claudeAgent
  • bun run test -- src/orchestration-v2/testkit/ClaudeReplayFixtures.integration.test.ts
  • bun run test -- src/orchestration-v2/testkit/ThreadFork.integration.test.ts -t Claude

Notes

  • Draft PR for review of current branch state. Codex all-provider replay still needs schema alignment with latest app-server behavior before it can be treated as a full-suite signal.

Note

Introduce orchestration V2 engine with run-centric thread lifecycle and new provider adapters

  • Adds a complete orchestration V2 system replacing session/turn concepts with runs, run attempts, and a runtime status model across server, client, and mobile; all providers (Claude, Codex, Cursor, Grok, OpenCode) now expose orchestrationAdapter via V2 adapter implementations
  • Introduces new DB schema via migrations 035–042 covering V2 event log, projection tables, effect outbox, scheduled tasks, and application event source; state directory changes from userdata to userdata-v2
  • Adds ThreadQueueControl, ThreadRelationshipsBanner, ThreadDetailsPanel, and QueuedRunsControl UI components for managing queued runs, relationships (fork/merge/subagent), and environment controls
  • Exposes MCP orchestrator and worktree toolkits (t3_thread_*, delegate_task, schedule_task, t3_worktree_handoff) via a new OrchestratorMcpService and WorktreeMcpService
  • Adds a Scheduled Tasks settings page and backend service with interval/fixed-time schedules, CRUD, and live subscription
  • Adds ACP Registry as a new provider driver kind (acpRegistry) with distribution resolution and platform-specific process ownership
  • Risk: Behavioral Change: existing cached orchestration data (shells, thread snapshots) stored under userdata will not be read; clients must reconnect to populate userdata-v2 caches. MCP session credentials no longer expire by time and must be explicitly revoked.

Macroscope summarized 408efa6.


Note

High Risk
Touches the production orchestration data model, client caches, and desktop on-disk state roots; wrong paths or projection semantics would break threads, approvals, and fresh installs without migrating legacy userdata.

Overview
Orchestration V2 application integration lands across clients and ops: mobile (and related shared runtime) now reads V2 shell/thread snapshots, runtime / runId instead of session/turn, and builds the chat from server visibleTurnItems with targeted projection joins for approvals, checkpoints, and tools.

New mobile thread capabilities include a queue control (reorder / promote-to-steer), lineage banner (fork/subagent/transfer navigation, merge-back, session detach), fork-from-assistant-response, expandable activity inspectors with rollback, and live vs stale handling when provider processes are gone for pending approvals/input.

Desktop production state moves from userdata to userdata-v2 with updated legacy Application Support naming (T3 Code (Alpha v2) / t3code-v2); tests and paths follow. CI installs build-essential so ACP process-tree fixture tests compile instead of soft-skipping. Plans document thread lineage/context transfer (19) and the full V2 cutover shapes (21). Minor copy: marketing Cursor tag, home “runtime” wording.

Reviewed by Cursor Bugbot for commit 408efa6. Bugbot is set up for automated code reviews on this repo. Configure here.

juliusmarminge and others added 30 commits April 17, 2026 17:29
Co-authored-by: codex <codex@users.noreply.github.com>
- Initialize provider as unchecked in a pending state
- Update initial probe message to reflect session-local status
- Type the runtime effect with `Scope`
- Build the ACP session runtime without wrapping it in `Effect.scoped`
- Use strict TurnId and ProviderItemId parsing in Codex session routing
- Decode in-memory stdio chunks in streaming mode to avoid split UTF-8 corruption
- Transfer session-owned scopes into adapter state
- Ensure runtime scopes close on stop and startup failure
- Add regression coverage for scoped lifecycle cleanup
- Close the managed native event logger when the adapter layer tears down
- Make session runtime close idempotent with an atomic closed flag
- Add coverage for flushing thread native logs on shutdown
- Use codex app-server snapshots for auth, models, and skills
- Remove legacy CLI/config discovery paths and related helpers
- Update tests for the new provider status flow
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
- Document the target orchestration graph, IDs, lifecycles, and capability model
- Add Codex app-server probe fixtures and update the probe test harness
- Introduce orchestration v2 service interfaces and error types
- Add replay runtime, fixtures, and integration coverage
- Update shared contracts and probe transcripts

Co-authored-by: codex <codex@users.noreply.github.com>
- Add Codex adapter and replay harness wiring
- Introduce in-memory orchestration projections and provider registry
- Expand orchestration contracts for turn and runtime events
Co-authored-by: codex <codex@users.noreply.github.com>
- Add context transfer IDs, schemas, and projections
- Support cheap fork creation and Codex native fork rollback
- Cover fork idempotency and replay behavior in tests
- Track remaining projection, context transfer, rollback, capability, and subagent work
- Clarify current V2 baseline and debugger-only follow-ups
- Map fork and merge-back turns into stored handoffs and transfer resolutions
- Add shell snapshot projection support plus coverage tests
- Update replay fixtures and web contracts for the new turn flow
Co-authored-by: codex <codex@users.noreply.github.com>
- Move Codex replay recording into `apps/server`
- Add Claude Agent SDK replay fixtures and test harness
- Update orchestration-v2 fixture scenarios and docs
- Move Claude provider runtime logic into its own module
- Share the SDK query runner between live and replay paths
- Add replay driver error wrapping for unexpected failures
Port orchestration V2 provider adapter wiring to the provider-instance driver registry.

Co-authored-by: codex <codex@users.noreply.github.com>
- persist the selected model on run records
- surface run model selection in the debug UI
- update replay fixtures and contracts for the new field
- Record Claude SDK transcripts across multiple prompts and restart/query modes
- Add approval and tool-call replay coverage for new orchestration fixtures
- Update Claude adapter testkit to model open/prompt/permission frames
- Derive Claude SDK query options from runtime policy
- Add read-only replay fixture and policy mapping tests
- Reuse shared approval-policy fixtures across orchestrator tests

Co-authored-by: codex <codex@users.noreply.github.com>
- add active steering and interrupt-restart replay fixtures
- update Claude adapter/orchestrator turn handling for steering
- refresh replay and integration test coverage
- add interrupt and mid-tool replay fixtures for Claude and Codex
- log Claude Agent SDK protocol frames to native event traces
- project Codex commandExecution start events into orchestration updates
- Map Cursor SDK agents and runs to V2 thread and turn lifecycles
- Update MCP capability, tool, and testing guidance for SDK-based injection
fieldCount: Object.keys(record).length,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High provider/NativeProtocolLogging.ts:33

In the object branch, Object.keys(record).length builds an array of every own enumerable key. A large untrusted payload with many fields therefore allocates memory proportional to the field count, which can exhaust the server while producing diagnostics that are supposed to be bounded. Consider counting keys without materializing the full array.

Suggested change
fieldCount: Object.keys(record).length,
fieldCount: Reflect.ownKeys(record).length,
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/provider/NativeProtocolLogging.ts around line 33:

In the object branch, `Object.keys(record).length` builds an array of every own enumerable key. A large untrusted payload with many fields therefore allocates memory proportional to the field count, which can exhaust the server while producing diagnostics that are supposed to be bounded. Consider counting keys without materializing the full array.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium

ThreadWorkLog's overflow toggle calls onToggleWorkGroup with the containing activity-group's entry.id, but onToggleWorkGroup hardcodes the anchor key as work-toggle:${groupId}. So disclosureAnchorKeyRef holds work-toggle:<entry.id>, which never matches any list item's actual key (entry.id). shouldRestoreVisibleContentPosition then rejects every visible item during the inline group expand/collapse, so the list fails to preserve the user's viewport and can jump as rows are inserted or removed. Use the activity-group entry ID directly as the anchor key instead of prefixing it.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/threads/ThreadFeed.tsx around line 1690:

`ThreadWorkLog`'s overflow toggle calls `onToggleWorkGroup` with the containing activity-group's `entry.id`, but `onToggleWorkGroup` hardcodes the anchor key as `work-toggle:${groupId}`. So `disclosureAnchorKeyRef` holds `work-toggle:<entry.id>`, which never matches any list item's actual `key` (`entry.id`). `shouldRestoreVisibleContentPosition` then rejects every visible item during the inline group expand/collapse, so the list fails to preserve the user's viewport and can jump as rows are inserted or removed. Use the activity-group entry ID directly as the anchor key instead of prefixing it.

latestRun,
runtime: shellRuntime(thread),
latestUserMessageAt: nullableIso(thread.latestUserMessageAt),
hasPendingApprovals:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low state/models.ts:171

presentThreadShell treats every pending request kind except user_input and auth_refresh as an approval, so a thread with pendingRuntimeRequest.kind === "dynamic_tool_call" reports hasPendingApprovals: true. That surfaces a false approval indicator even though there is no approval the user can open or answer. Consider excluding dynamic_tool_call (and any other non-approval kinds) from the hasPendingApprovals check, matching the exclusion already applied elsewhere.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/client-runtime/src/state/models.ts around line 171:

`presentThreadShell` treats every pending request kind except `user_input` and `auth_refresh` as an approval, so a thread with `pendingRuntimeRequest.kind === "dynamic_tool_call"` reports `hasPendingApprovals: true`. That surfaces a false approval indicator even though there is no approval the user can open or answer. Consider excluding `dynamic_tool_call` (and any other non-approval kinds) from the `hasPendingApprovals` check, matching the exclusion already applied elsewhere.

);

const rawResult = (yield* Ref.get(outputRef)).trim();
const rawResult = promptResult.result?.trim() ?? "";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium textGeneration/CursorTextGeneration.ts:126

When the Cursor SDK returns status: "cancelled" or status: "error" but still populates result with partial text, runCursorJson proceeds to decode that text and returns it as a successful generation. The status check on line 126 only guards against empty result, so non-empty partial output from a cancelled or failed request bypasses the error path. Consider rejecting any non-finished status before inspecting promptResult.result.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/textGeneration/CursorTextGeneration.ts around line 126:

When the Cursor SDK returns `status: "cancelled"` or `status: "error"` but still populates `result` with partial text, `runCursorJson` proceeds to decode that text and returns it as a successful generation. The status check on line 126 only guards against empty `result`, so non-empty partial output from a cancelled or failed request bypasses the error path. Consider rejecting any non-`finished` status before inspecting `promptResult.result`.

input.strategy === "full_thread_summary"
? "Full conversation context for provider handoff."
: "Conversation delta since this provider last participated.",
`From driver: ${input.fromProviderInstanceId}`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low orchestration-v2/ContextHandoffService.ts:147

makeProviderHandoffSummary labels fromProviderInstanceId and toProviderInstanceId as "From driver" / "To driver", but these are user-defined provider instance IDs (routing keys), not driver kinds. The handoff summary text therefore reports incorrect provider identity metadata to the model whenever an instance ID doesn't match its driver kind. Consider labeling these as provider instances, or passing and displaying the actual driver kinds.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/ContextHandoffService.ts around line 147:

`makeProviderHandoffSummary` labels `fromProviderInstanceId` and `toProviderInstanceId` as "From driver" / "To driver", but these are user-defined provider instance IDs (routing keys), not driver kinds. The handoff summary text therefore reports incorrect provider identity metadata to the model whenever an instance ID doesn't match its driver kind. Consider labeling these as provider instances, or passing and displaying the actual driver kinds.

Comment on lines +62 to +70
const activeRun =
runsByOrdinal.find(
(run) =>
run.status === "preparing" ||
run.status === "queued" ||
run.status === "starting" ||
run.status === "running" ||
run.status === "waiting",
) ?? null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium state/use-thread-selection.ts:62

threadDetailToShell includes queued in the active-run lookup, so when a later run is queued while an earlier run is still running, it sets activeRunId to the queued run and reports its status — hiding the actually running run until the shell list materializes. The canonical shell derivation excludes queued from active runs and always derives status from the latest run. Remove queued from the active-run predicate and use latestRun?.status for status.

Suggested change
const activeRun =
runsByOrdinal.find(
(run) =>
run.status === "preparing" ||
run.status === "queued" ||
run.status === "starting" ||
run.status === "running" ||
run.status === "waiting",
) ?? null;
const activeRun =
runsByOrdinal.find(
(run) =>
run.status === "preparing" ||
run.status === "starting" ||
run.status === "running" ||
run.status === "waiting",
) ?? null;
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/state/use-thread-selection.ts around lines 62-70:

`threadDetailToShell` includes `queued` in the active-run lookup, so when a later run is `queued` while an earlier run is still running, it sets `activeRunId` to the queued run and reports its status — hiding the actually running run until the shell list materializes. The canonical shell derivation excludes `queued` from active runs and always derives `status` from the latest run. Remove `queued` from the active-run predicate and use `latestRun?.status` for `status`.

commandId: yield* commandId(input),
});
});
export const updateThreadMetadata = Effect.fn("EnvironmentCommands.updateThreadMetadata")(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium operations/commands.ts:360

updateThreadMetadata dispatches two separate commands for one logical update. When the first command (metadata fields) succeeds but the projection fetch or the second command (model selection / provider switch) fails, the effect reports failure even though the first mutation was already committed. A retry then produces a partially applied update rather than the all-or-nothing behavior the API implies. Consider combining the mutation server-side or returning an explicit partial-result type so callers know the first half succeeded.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/client-runtime/src/operations/commands.ts around line 360:

`updateThreadMetadata` dispatches two separate commands for one logical update. When the first command (metadata fields) succeeds but the projection fetch or the second command (model selection / provider switch) fails, the effect reports failure even though the first mutation was already committed. A retry then produces a partially applied update rather than the all-or-nothing behavior the API implies. Consider combining the mutation server-side or returning an explicit partial-result type so callers know the first half succeeded.

@@ -247,8 +251,42 @@ export const BranchToolbar = memo(function BranchToolbar({

if (!hasActiveThread || !activeProject) return null;

if (layout === "panel") {
return (
<div className="flex w-full flex-col" data-thread-panel-run-context>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium components/BranchToolbar.tsx:256

When layout="panel" and only a single non-primary environment is available, the panel renders workspace and branch rows but never renders any environment indicator. The caller in ThreadDetailsPanel only adds a separate BranchToolbarEnvironmentSelector when availableEnvironments.length > 1, so the single-remote-environment case — the exact scenario shouldShowEnvironmentIndicator exists to surface — is silently hidden in panel mode. In composer mode the same case is handled by the showEnvironmentIndicator branch, but the panel return path skips it entirely. Consider rendering the environment indicator (either inline or via BranchToolbarEnvironmentSelector) in the panel layout as well.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/BranchToolbar.tsx around line 256:

When `layout="panel"` and only a single non-primary environment is available, the panel renders workspace and branch rows but never renders any environment indicator. The caller in `ThreadDetailsPanel` only adds a separate `BranchToolbarEnvironmentSelector` when `availableEnvironments.length > 1`, so the single-remote-environment case — the exact scenario `shouldShowEnvironmentIndicator` exists to surface — is silently hidden in panel mode. In `composer` mode the same case is handled by the `showEnvironmentIndicator` branch, but the panel return path skips it entirely. Consider rendering the environment indicator (either inline or via `BranchToolbarEnvironmentSelector`) in the panel layout as well.

Comment thread apps/web/src/components/ChatView.logic.ts
Comment on lines +209 to +211
await this.changed(signal);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High Adapters/OpenCodeAdapterV2.testkit.ts:209

The events async generator hangs forever when the transcript entries are exhausted without a trailing runtime_exit entry. The loop calls await this.changed(signal) and blocks indefinitely because nothing ever advances the cursor or resolves the waiter. The schema permits an empty or truncated entries array, and assertComplete() treats EOF as complete, so this scenario is reachable. When this.cursor >= this.transcript.entries.length, the generator should return instead of waiting.

      await this.changed(signal);
+      if (this.cursor >= this.transcript.entries.length) {
+        return;
+      }
    }
  }
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.testkit.ts around lines 209-211:

The `events` async generator hangs forever when the transcript entries are exhausted without a trailing `runtime_exit` entry. The loop calls `await this.changed(signal)` and blocks indefinitely because nothing ever advances the cursor or resolves the waiter. The schema permits an empty or truncated `entries` array, and `assertComplete()` treats EOF as complete, so this scenario is reachable. When `this.cursor >= this.transcript.entries.length`, the generator should return instead of waiting.

Comment on lines +306 to +308
const nativeEventLogger = yield* makeEventNdjsonLogger(providerEventLogPath, {
stream: "native",
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low Adapters/CursorAgentSdk.ts:306

The layer creates a batched EventNdjsonLogger (200 ms flush window) but never calls its close() method — assertComplete is Effect.void, so on layer shutdown the per-thread writer scopes stay open and any records still in the batch window are dropped. Consider yielding nativeEventLogger.close() via Effect.addFinalizer so pending records are flushed before the layer disposes.

      const nativeEventLogger = yield* makeEventNdjsonLogger(providerEventLogPath, {
        stream: "native",
      });
+     const scope = yield* Effect.scope();
+     yield* Effect.addFinalizer(scope, () => nativeEventLogger.close());
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Adapters/CursorAgentSdk.ts around lines 306-308:

The layer creates a batched `EventNdjsonLogger` (200 ms flush window) but never calls its `close()` method — `assertComplete` is `Effect.void`, so on layer shutdown the per-thread writer scopes stay open and any records still in the batch window are dropped. Consider yielding `nativeEventLogger.close()` via `Effect.addFinalizer` so pending records are flushed before the layer disposes.

@juliusmarminge
juliusmarminge force-pushed the t3code/codex-turn-mapping branch from 6b1c011 to cd04ad1 Compare July 24, 2026 13:09
(attachment): attachment is UploadChatAttachment => "dataUrl" in attachment,
);
if (uploads.length === 0) return stored;
const result = yield* request(WS_METHODS.assetsPersistChatAttachments, {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High operations/commands.ts:204

persistAttachments sends only the filtered uploads array to assetsPersistChatAttachments, so the server computes each persisted attachment's ID from its index within that filtered list. When a message mixes already-stored attachments with new uploads, the uploads are renumbered starting at 0, which can collide with an existing stored attachment's ${messageId}:${index} ID — overwriting the stored file and producing duplicate attachment IDs in the returned message. Consider sending the original indices alongside the uploads (or including placeholders for stored attachments) so the server derives non-colliding IDs.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/client-runtime/src/operations/commands.ts around line 204:

`persistAttachments` sends only the filtered `uploads` array to `assetsPersistChatAttachments`, so the server computes each persisted attachment's ID from its index within that filtered list. When a message mixes already-stored attachments with new uploads, the uploads are renumbered starting at 0, which can collide with an existing stored attachment's `${messageId}:${index}` ID — overwriting the stored file and producing duplicate attachment IDs in the returned message. Consider sending the original indices alongside the uploads (or including placeholders for stored attachments) so the server derives non-colliding IDs.

providerThreadId === null
? null
: (projection.providerThreads.find((thread) => thread.id === providerThreadId) ?? null);
const attachedProviderThread =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium state/threadWorkflows.ts:37

When no active provider thread is resolved from the active run, resolveThreadProviderSession picks the first provider thread attached to the app thread via find, ignoring thread status. Because historical provider threads persist across handoffs and forks, this can attach a stopped or errored provider thread whose session is stale — causing queue/steering and detach actions to be gated by the wrong capabilities or disabled entirely. Consider filtering for provider threads whose session is not stopped/error, or selecting the most recently active thread before resolving its session.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/client-runtime/src/state/threadWorkflows.ts around line 37:

When no active provider thread is resolved from the active run, `resolveThreadProviderSession` picks the first provider thread attached to the app thread via `find`, ignoring thread status. Because historical provider threads persist across handoffs and forks, this can attach a stopped or errored provider thread whose session is stale — causing queue/steering and detach actions to be gated by the wrong capabilities or disabled entirely. Consider filtering for provider threads whose session is not `stopped`/`error`, or selecting the most recently active thread before resolving its session.

}
}

async response(operation: string): Promise<unknown> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/OpenCodeAdapterV2.testkit.ts:151

In OpenCodeReplayController.response, replies are correlated only by frame.operation, so concurrent requests for the same SDK operation can receive each other's data. For a transcript ordered as outbound A, outbound B, response A, response B: request A waits at outbound B, then request B consumes response A (its operation matches), and A consumes response B after being awakened — silently swapping replay results and making concurrent same-operation replays incorrect. Consider correlating by per-request ordering (e.g., matching the Nth response for an operation to the Nth pending request) instead of matching only frame.operation.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.testkit.ts around line 151:

In `OpenCodeReplayController.response`, replies are correlated only by `frame.operation`, so concurrent requests for the same SDK operation can receive each other's data. For a transcript ordered as outbound A, outbound B, response A, response B: request A waits at outbound B, then request B consumes response A (its `operation` matches), and A consumes response B after being awakened — silently swapping replay results and making concurrent same-operation replays incorrect. Consider correlating by per-request ordering (e.g., matching the Nth response for an operation to the Nth pending request) instead of matching only `frame.operation`.

Comment thread apps/server/src/orchestration-v2/ShellStream.ts
Comment thread apps/server/src/ws.ts
},
diff,
);
const turnDiff = yield* getTurnDiff({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low checkpointing/CheckpointDiffQuery.ts:228

When getFullThreadDiff calls getTurnDiff for a range that is unavailable, the error returned (e.g. CheckpointTurnRangeUnavailableError) has its operation field set to CheckpointDiffQuery.getTurnDiff instead of CheckpointDiffQuery.getFullThreadDiff, producing misleading error messages and telemetry that identify the wrong RPC. This happens because getFullThreadDiff delegates to getTurnDiff, which hardcodes const operation = "CheckpointDiffQuery.getTurnDiff" and uses it for every error it constructs. Consider accepting an operation parameter on getTurnDiff so callers can supply the correct operation context.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/checkpointing/CheckpointDiffQuery.ts around line 228:

When `getFullThreadDiff` calls `getTurnDiff` for a range that is unavailable, the error returned (e.g. `CheckpointTurnRangeUnavailableError`) has its `operation` field set to `CheckpointDiffQuery.getTurnDiff` instead of `CheckpointDiffQuery.getFullThreadDiff`, producing misleading error messages and telemetry that identify the wrong RPC. This happens because `getFullThreadDiff` delegates to `getTurnDiff`, which hardcodes `const operation = "CheckpointDiffQuery.getTurnDiff"` and uses it for every error it constructs. Consider accepting an `operation` parameter on `getTurnDiff` so callers can supply the correct operation context.

@juliusmarminge
juliusmarminge force-pushed the t3code/codex-turn-mapping branch from cd04ad1 to 1e58e65 Compare July 24, 2026 13:31
Co-authored-by: codex <codex@users.noreply.github.com>
@juliusmarminge
juliusmarminge force-pushed the t3code/codex-turn-mapping branch from 1e58e65 to a286c60 Compare July 24, 2026 13:38
].join("\n");
}

function makeProviderHandoffSummary(input: {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium orchestration-v2/ContextHandoffService.ts:126

makeProviderHandoffSummary includes every turn item with no size or item-count limit. When strategy is full_thread_summary and the caller passes the entire projection.turnItems history, a long-lived thread produces an arbitrarily large handoff summary that is prepended directly to the next provider message. This unbounded summary can exceed the provider's context window, preventing the resume-fallback run from starting instead of providing a bounded summary. Consider capping the number of summarized items or the total summary text length, similar to the compactText per-item limit, so the handoff stays within context-window bounds.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/ContextHandoffService.ts around line 126:

`makeProviderHandoffSummary` includes every turn item with no size or item-count limit. When `strategy` is `full_thread_summary` and the caller passes the entire `projection.turnItems` history, a long-lived thread produces an arbitrarily large handoff summary that is prepended directly to the next provider message. This unbounded summary can exceed the provider's context window, preventing the resume-fallback run from starting instead of providing a bounded summary. Consider capping the number of summarized items or the total summary text length, similar to the `compactText` per-item limit, so the handoff stays within context-window bounds.

return yield* fs.realPath(cwd);
}).pipe(Effect.provide(NodeServices.layer)),
);
const pathFragments = [

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium scripts/probe-claude-fork-local-rollback-replay.ts:224

sanitizeValue does unrestricted substring replacement, so when cwd is a relative path like . (the default from makeCheckpointWorkspace), every period in every transcript string gets replaced with the synthetic directory. This silently corrupts version numbers, prose, and filenames throughout the generated replay fixture instead of only normalizing filesystem paths. Consider resolving cwd to an absolute path before building pathFragments, or skip fragments that are too short/ambiguous to be safe for global substring replacement.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/scripts/probe-claude-fork-local-rollback-replay.ts around line 224:

`sanitizeValue` does unrestricted substring replacement, so when `cwd` is a relative path like `.` (the default from `makeCheckpointWorkspace`), every period in every transcript string gets replaced with the synthetic directory. This silently corrupts version numbers, prose, and filenames throughout the generated replay fixture instead of only normalizing filesystem paths. Consider resolving `cwd` to an absolute path before building `pathFragments`, or skip fragments that are too short/ambiguous to be safe for global substring replacement.

return String(error);
}

const decodeRow = (row: ScheduledTaskRow) =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium scheduledTasks/ScheduledTaskService.ts:124

decodeRow calls ScheduledTaskId.make(row.task_id) outside the schema-decode pipeline. When task_id is corrupt (empty or whitespace), this throws a defect instead of producing the typed ScheduledTaskError that Effect.mapError wraps. The lenient scheduler uses Effect.result(decodeRow(row)), which does not catch defects — so a single malformed row aborts the entire poll loop instead of being skipped and logged. The mapError handler also repeats ScheduledTaskId.make(row.task_id), which can throw the same defect. Consider constructing ScheduledTaskId inside the schema (or within the generator body) and passing the raw row.task_id into taskError so both the decode failure and the error handler stay defect-free.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/scheduledTasks/ScheduledTaskService.ts around line 124:

`decodeRow` calls `ScheduledTaskId.make(row.task_id)` outside the schema-decode pipeline. When `task_id` is corrupt (empty or whitespace), this throws a defect instead of producing the typed `ScheduledTaskError` that `Effect.mapError` wraps. The lenient scheduler uses `Effect.result(decodeRow(row))`, which does not catch defects — so a single malformed row aborts the entire poll loop instead of being skipped and logged. The `mapError` handler also repeats `ScheduledTaskId.make(row.task_id)`, which can throw the same defect. Consider constructing `ScheduledTaskId` inside the schema (or within the generator body) and passing the raw `row.task_id` into `taskError` so both the decode failure and the error handler stay defect-free.

{item.type === "file_search" && item.results ? (
<ul className="space-y-1 rounded-md border border-border/45 p-2">
{item.results.map((result) => (
<li key={JSON.stringify(result)}>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low chat/V2ItemInspector.tsx:176

The file_search and web_search result lists use JSON.stringify(result) as the React key. When the provider returns duplicate result objects, the sibling keys collide, which causes React reconciliation to reuse or render the wrong row on subsequent updates. The key should include the array index or another occurrence-unique value.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/V2ItemInspector.tsx around line 176:

The `file_search` and `web_search` result lists use `JSON.stringify(result)` as the React `key`. When the provider returns duplicate result objects, the sibling keys collide, which causes React reconciliation to reuse or render the wrong row on subsequent updates. The key should include the array index or another occurrence-unique value.

const WorkGroupSection = memo(function WorkGroupSection({
groupedEntries,
}: {
groupedEntries: Extract<MessagesTimelineRow, { kind: "work" }>["groupedEntries"];
}) {
const { workspaceRoot } = use(TimelineRowCtx);
const [isExpanded, setIsExpanded] = useState(false);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low chat/MessagesTimeline.tsx:1523

WorkGroupSection keeps its isExpanded state in a local useState, so when LegendList virtualizes the row away and later remounts it, the expanded state resets to false. A user who expands a work group, scrolls away, and scrolls back sees the group collapsed again, losing their interaction. Previously this state lived in the parent (expandedWorkGroupIds) and survived row virtualization. Consider lifting the expanded-work-group state back to the parent so it persists across mount/unmount cycles.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MessagesTimeline.tsx around line 1523:

`WorkGroupSection` keeps its `isExpanded` state in a local `useState`, so when `LegendList` virtualizes the row away and later remounts it, the expanded state resets to `false`. A user who expands a work group, scrolls away, and scrolls back sees the group collapsed again, losing their interaction. Previously this state lived in the parent (`expandedWorkGroupIds`) and survived row virtualization. Consider lifting the expanded-work-group state back to the parent so it persists across mount/unmount cycles.

readonly state: Ref.Ref<CodexAppServerReplayState>;
}

const encoder = new TextEncoder();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low src/replay.ts:149

The module-global decoder in replay.ts is shared across all replay clients. Because processOutboundChunk calls decoder.decode(chunk, { stream: true }), the streaming decoder retains partial multibyte state between calls. When two replay clients run concurrently and their UTF-8 chunks split a multibyte character, they corrupt each other's decoder state, producing spurious JSON parse or frame-mismatch failures. Each client should create its own TextDecoder inside makeReplayClientWithState instead of using the module-global decoder.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/effect-codex-app-server/src/replay.ts around line 149:

The module-global `decoder` in `replay.ts` is shared across all replay clients. Because `processOutboundChunk` calls `decoder.decode(chunk, { stream: true })`, the streaming decoder retains partial multibyte state between calls. When two replay clients run concurrently and their UTF-8 chunks split a multibyte character, they corrupt each other's decoder state, producing spurious JSON parse or frame-mismatch failures. Each client should create its own `TextDecoder` inside `makeReplayClientWithState` instead of using the module-global `decoder`.

Comment thread apps/server/src/ws.ts
Comment on lines +187 to +197
yield* fileSystem
.writeFile(path.join(config.attachmentsDir, attachmentRelativePath(persisted)), bytes)
.pipe(
Effect.mapError(
(cause) =>
new PersistChatAttachmentsError({
message: `Could not persist attachment ${attachment.name}.`,
cause,
}),
),
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High src/ws.ts:187

persistChatAttachments uses writeFile with an exclusive-create flag, so re-sending the same RPC (same threadId, messageId, and attachment index) overwrites the file on disk that an already-stored message references, silently altering historical message content. The deterministic path means re-persisting the same logical attachment always targets the same file. Consider rejecting writes to an existing destination (e.g., an exclusive create that fails if the file exists, or comparing existing bytes) so prior message content cannot be changed.

-      yield* fileSystem
-        .writeFile(path.join(config.attachmentsDir, attachmentRelativePath(persisted)), bytes)
+      yield* fileSystem
+        .writeFile(path.join(config.attachmentsDir, attachmentRelativePath(persisted)), bytes, { flag: "wx" })
+        .pipe(
+          Effect.catchTag("FileSystemError", (cause) =>
+            new PersistChatAttachmentsError({
+              message: `Attachment ${attachment.name} already exists.`,
+              cause,
+            }),
+          ),
+          Effect.mapError(
+            (cause) =>
+              new PersistChatAttachmentsError({
+                message: `Could not persist attachment ${attachment.name}.`,
+                cause,
+              }),
+          ),
+        );
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/ws.ts around lines 187-197:

`persistChatAttachments` uses `writeFile` with an exclusive-create flag, so re-sending the same RPC (same `threadId`, `messageId`, and attachment index) overwrites the file on disk that an already-stored message references, silently altering historical message content. The deterministic path means re-persisting the same logical attachment always targets the same file. Consider rejecting writes to an existing destination (e.g., an exclusive create that fails if the file exists, or comparing existing bytes) so prior message content cannot be changed.

const crypto = yield* Crypto.Crypto;
const idAllocator = yield* IdAllocatorV2;
const serverConfig = yield* ServerConfig;
const replayDir = yield* fileSystem

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low Adapters/GrokAdapterV2.testkit.ts:41

The replay layer creates replayDir via fileSystem.makeTempDirectory but never cleans it up on scope finalization, so each replay run leaves a temp directory (containing status.json) behind in the OS temp area. These directories accumulate across repeated test runs. Consider scoping the temp directory so it is removed when the layer's scope closes.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Adapters/GrokAdapterV2.testkit.ts around line 41:

The replay layer creates `replayDir` via `fileSystem.makeTempDirectory` but never cleans it up on scope finalization, so each replay run leaves a temp directory (containing `status.json`) behind in the OS temp area. These directories accumulate across repeated test runs. Consider scoping the temp directory so it is removed when the layer's scope closes.

Comment on lines +40 to +42
const failedTurn = projection.providerTurns.find(
(turn) => turn.status !== "completed" && turn.status !== "running",
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low claude_result_is_error/output.ts:40

failedTurn is selected as any provider turn whose status is not completed or running, without linking it to failedRun. If run 1's own provider turn actually completed while a different run's provider turn failed, this assertion still passes — it never verifies that run 1's provider turn is the failed one, despite the comment stating that requirement. Consider filtering failedTurn by failedRun?.id (e.g. matching turn.runId === failedRun?.id) so the assertion checks the correct turn.

Suggested change
const failedTurn = projection.providerTurns.find(
(turn) => turn.status !== "completed" && turn.status !== "running",
);
const failedTurn = projection.providerTurns.find(
(turn) => turn.runId === failedRun?.id && turn.status !== "completed" && turn.status !== "running",
);
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/testkit/fixtures/claude_result_is_error/output.ts around lines 40-42:

`failedTurn` is selected as *any* provider turn whose status is not `completed` or `running`, without linking it to `failedRun`. If run 1's own provider turn actually completed while a *different* run's provider turn failed, this assertion still passes — it never verifies that run 1's provider turn is the failed one, despite the comment stating that requirement. Consider filtering `failedTurn` by `failedRun?.id` (e.g. matching `turn.runId === failedRun?.id`) so the assertion checks the correct turn.

resumeQueuedRuns: Effect.fail(
new OrchestratorDispatchError({
commandId: CommandId.make("command:system:resume-queued-runs"),
commandType: "message.dispatch",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low orchestration-v2/Orchestrator.ts:5339

layerUnavailable.resumeQueuedRuns fails with an OrchestratorDispatchError whose commandType is "message.dispatch", so any log or UI surface that renders commandType misidentifies the failed operation as a message dispatch instead of the system resume-queued-runs action. The fallback should use a commandType that reflects the actual resume operation so the error is distinguishable.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Orchestrator.ts around line 5339:

`layerUnavailable.resumeQueuedRuns` fails with an `OrchestratorDispatchError` whose `commandType` is `"message.dispatch"`, so any log or UI surface that renders `commandType` misidentifies the failed operation as a message dispatch instead of the system resume-queued-runs action. The fallback should use a `commandType` that reflects the actual resume operation so the error is distinguishable.

)
? null
: yield* checkpoints.materializeBaselineCheckpoint({ scope });
const checkpoint = yield* checkpoints.capture({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High orchestration-v2/CheckpointCaptureService.ts:99

When checkpoints.capture returns a checkpoint with status: "error" (VCS capture failure), execute still commits that checkpoint and marks the run and node as "completed" with its checkpointId. On any retry, the early idempotency check sees a completed run with a non-null checkpointId and returns immediately, so the failed checkpoint is never recaptured. The stored checkpoint is also unrestorable — CheckpointServiceV2.restore rejects non-ready checkpoints — leaving the run permanently stuck. After calling checkpoints.capture, the code should inspect the returned checkpoint's status and avoid committing a completion event when it is not ready.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/CheckpointCaptureService.ts around line 99:

When `checkpoints.capture` returns a checkpoint with `status: "error"` (VCS capture failure), `execute` still commits that checkpoint and marks the run and node as `"completed"` with its `checkpointId`. On any retry, the early idempotency check sees a completed run with a non-null `checkpointId` and returns immediately, so the failed checkpoint is never recaptured. The stored checkpoint is also unrestorable — `CheckpointServiceV2.restore` rejects non-`ready` checkpoints — leaving the run permanently stuck. After calling `checkpoints.capture`, the code should inspect the returned checkpoint's `status` and avoid committing a completion event when it is not `ready`.

return parseCgroup2Mounts(contents)[0];
}

export function wrapCommandForLinuxCgroup(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium acp/AcpSessionRuntime.ts:214

When cgroup containment is enabled, wrapCommandForLinuxCgroup spawns the command via process.execve, which drops the normal execvp/posix_spawn fallback that retries through /bin/sh on ENOEXEC. A provider command that is a text script without a recognizable shebang (no leading #!) works normally but fails under cgroup containment because execve returns ENOEXEC and the wrapper exits 125. Consider falling back to /bin/sh -c on ENOEXEC before calling process.execve, or document that cgroup-contained commands must have a valid shebang or be directly executable.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/provider/acp/AcpSessionRuntime.ts around line 214:

When cgroup containment is enabled, `wrapCommandForLinuxCgroup` spawns the command via `process.execve`, which drops the normal `execvp`/`posix_spawn` fallback that retries through `/bin/sh` on `ENOEXEC`. A provider command that is a text script without a recognizable shebang (no leading `#!`) works normally but fails under cgroup containment because `execve` returns `ENOEXEC` and the wrapper exits 125. Consider falling back to `/bin/sh -c` on `ENOEXEC` before calling `process.execve`, or document that cgroup-contained commands must have a valid shebang or be directly executable.

}
}

async *events(signal?: AbortSignal): AsyncIterable<OpenCodeEvent> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low Adapters/OpenCodeAdapterV2.testkit.ts:180

The events async generator emits an event after the subscription has been aborted. The signal is only checked at the top of each loop iteration, so if an abort arrives during the Effect.sleep(Duration.millis(entry.afterMs)) delay, the generator still advances the cursor and yields the event instead of stopping. This also means any teardown waiting on the stream to end can remain blocked for the full afterMs duration. Consider awaiting the sleep with an AbortSignal race (or re-checking signal?.aborted after the delay, before yielding) so an abort during the delay stops the generator immediately.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.testkit.ts around line 180:

The `events` async generator emits an event **after** the subscription has been aborted. The signal is only checked at the top of each loop iteration, so if an abort arrives during the `Effect.sleep(Duration.millis(entry.afterMs))` delay, the generator still advances the cursor and yields the event instead of stopping. This also means any teardown waiting on the stream to end can remain blocked for the full `afterMs` duration. Consider awaiting the sleep with an `AbortSignal` race (or re-checking `signal?.aborted` after the delay, before yielding) so an abort during the delay stops the generator immediately.

if (input.fixtureInput.steps[stepIndex + 1]?.type !== "approve_next_runtime_request") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium fixtures/shared.ts:571

When a steer or restart step is immediately followed by answer_next_user_input_request, the generated timeline pushes await_thread_idle before the answer step. The run cannot become idle until the user-input answer is sent, so the fixture deadlocks and times out. The follow-up check at line 571 and the matching check at line 605 only skip the idle-wait for approve_next_runtime_request; they need to also skip it for answer_next_user_input_request.

-          if (input.fixtureInput.steps[stepIndex + 1]?.type !== "approve_next_runtime_request") {
+          if (
+            input.fixtureInput.steps[stepIndex + 1]?.type !== "approve_next_runtime_request" &&
+            input.fixtureInput.steps[stepIndex + 1]?.type !== "answer_next_user_input_request"
+          ) {
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/testkit/fixtures/shared.ts around line 571:

When a `steer` or `restart` step is immediately followed by `answer_next_user_input_request`, the generated timeline pushes `await_thread_idle` before the answer step. The run cannot become idle until the user-input answer is sent, so the fixture deadlocks and times out. The follow-up check at line 571 and the matching check at line 605 only skip the idle-wait for `approve_next_runtime_request`; they need to also skip it for `answer_next_user_input_request`.

Comment on lines +111 to +113
const runsToRollback = projection.runs.filter(
(run) => run.ordinal > targetOrdinal && run.status === "completed",
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium orchestration-v2/CheckpointRollbackService.ts:111

runsToRollback filters only runs with status === "completed", so runs that are failed, cancelled, or interrupted are silently excluded from the rollback events. The filesystem checkpoint is restored, but these terminal runs (and their nodes) are never marked rolled_back in the event log, leaving stale state that incorrectly appears to be after the target checkpoint. Additionally, when all runs after the target ordinal are non-completed terminal runs, runsToRollback.length === 0 evaluates true and session.rollbackThread is skipped entirely — the provider conversation is not rolled back even though the filesystem was. Consider including all terminal run statuses (or all runs with ordinal > targetOrdinal) so the persisted run state and provider thread are both rolled back consistently.

       const runsToRollback = projection.runs.filter(
-        (run) => run.ordinal > targetOrdinal && run.status === "completed",
+        (run) => run.ordinal > targetOrdinal && (run.status === "completed" || run.status === "failed" || run.status === "cancelled" || run.status === "interrupted"),
       );
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/CheckpointRollbackService.ts around lines 111-113:

`runsToRollback` filters only runs with `status === "completed"`, so runs that are `failed`, `cancelled`, or `interrupted` are silently excluded from the rollback events. The filesystem checkpoint is restored, but these terminal runs (and their nodes) are never marked `rolled_back` in the event log, leaving stale state that incorrectly appears to be after the target checkpoint. Additionally, when all runs after the target ordinal are non-completed terminal runs, `runsToRollback.length === 0` evaluates true and `session.rollbackThread` is skipped entirely — the provider conversation is not rolled back even though the filesystem was. Consider including all terminal run statuses (or all runs with `ordinal > targetOrdinal`) so the persisted run state and provider thread are both rolled back consistently.

threadId: node.threadId,
runId: node.runId ?? input.run.id,
nodeId: node.id,
providerInstanceId: input.run.providerInstanceId,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium orchestration-v2/RunExecutionService.ts:211

The cascade in cascadeTerminalizeRunOwnedSubagents writes every child-thread node.updated event with input.run.providerInstanceId instead of the node's own node.providerInstanceId. Child subagent nodes can run on a different provider instance than the root run, so this stamps the wrong provider instance on the event's top-level providerInstanceId field. Instance-indexed event logs or instance-scoped readers will see the node attributed to the wrong provider. Consider using node.providerInstanceId for the cascaded node.updated event (and the turn item's own instance identity where available).

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/RunExecutionService.ts around line 211:

The cascade in `cascadeTerminalizeRunOwnedSubagents` writes every child-thread `node.updated` event with `input.run.providerInstanceId` instead of the node's own `node.providerInstanceId`. Child subagent nodes can run on a different provider instance than the root run, so this stamps the wrong provider instance on the event's top-level `providerInstanceId` field. Instance-indexed event logs or instance-scoped readers will see the node attributed to the wrong provider. Consider using `node.providerInstanceId` for the cascaded `node.updated` event (and the turn item's own instance identity where available).

Comment thread apps/server/src/provider/acp/XAiAcpExtension.ts
...readArgValues("--scenario"),
...(process.env.T3_CODEX_REPLAY_SCENARIOS ? [process.env.T3_CODEX_REPLAY_SCENARIOS] : []),
];
const requested = rawValues.length > 0 ? rawValues : ["simple"];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low scripts/record-codex-app-server-replay-fixture.ts:188

parseScenarios silently falls back to recording the simple fixture when --scenario is supplied with no following value, instead of rejecting the malformed CLI invocation. For example, a command ending in --scenario overwrites the simple fixture rather than erroring. This happens because readArgValues drops any --scenario occurrence that lacks a following value, so rawValues is empty and line 188 substitutes ["simple"]. Consider treating a valueless --scenario as a CLI error.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/scripts/record-codex-app-server-replay-fixture.ts around line 188:

`parseScenarios` silently falls back to recording the `simple` fixture when `--scenario` is supplied with no following value, instead of rejecting the malformed CLI invocation. For example, a command ending in `--scenario` overwrites the `simple` fixture rather than erroring. This happens because `readArgValues` drops any `--scenario` occurrence that lacks a following value, so `rawValues` is empty and line 188 substitutes `["simple"]`. Consider treating a valueless `--scenario` as a CLI error.

<SimpleWorkEntryRow
key={workEntry.id}
workEntry={workEntry}
workspaceRoot={workspaceRoot}
/>
))}
</div>
{hasOverflow && (
<button

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low chat/MessagesTimeline.tsx:1594

The expand/collapse button in WorkGroupSection is missing the aria-expanded attribute. Screen-reader users can activate the "Show more/fewer" toggle, but the button does not expose whether the group is currently expanded or collapsed, so the control's state is invisible to assistive technology. Consider adding aria-expanded={isExpanded} to the button.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MessagesTimeline.tsx around line 1594:

The expand/collapse button in `WorkGroupSection` is missing the `aria-expanded` attribute. Screen-reader users can activate the "Show more/fewer" toggle, but the button does not expose whether the group is currently expanded or collapsed, so the control's state is invisible to assistive technology. Consider adding `aria-expanded={isExpanded}` to the button.

Comment on lines +74 to +80
Effect.catchCause((cause) =>
Effect.logWarning("orchestration-v2.provider-continuation.dispatch-failed", {
threadId: request.threadId,
providerThreadId: request.providerThreadId,
cause,
}),
),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium orchestration-v2/ProviderContinuationService.ts:74

When dispatchContinuation fails before reaching request.dispatchIfCurrent (e.g. getThreadProjection or ID allocation throws), the worker catches and logs the error but never calls request.clearIfCurrent. The adapter's sticky continuationRequested flag stays set, so the adapter refuses to offer another request and the provider continuation is permanently stranded until the session is reset. Consider calling request.clearIfCurrent (or request.dispatchIfCurrent(Effect.void)) in the catchCause handler before logging so the adapter's sticky flag is cleared on failure.

          Effect.catchCause((cause) =>
+            Effect.gen(function* () {
+              if (request.clearIfCurrent !== undefined) {
+                yield* request.clearIfCurrent();
+              } else if (request.dispatchIfCurrent !== undefined) {
+                yield* request.dispatchIfCurrent(Effect.void);
+              }
+            }).pipe(
+              Effect.catchAll(() => Effect.void),
              Effect.logWarning("orchestration-v2.provider-continuation.dispatch-failed", {
                threadId: request.threadId,
                providerThreadId: request.providerThreadId,
                cause,
              }),
+            ),
          ),
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/ProviderContinuationService.ts around lines 74-80:

When `dispatchContinuation` fails before reaching `request.dispatchIfCurrent` (e.g. `getThreadProjection` or ID allocation throws), the worker catches and logs the error but never calls `request.clearIfCurrent`. The adapter's sticky `continuationRequested` flag stays set, so the adapter refuses to offer another request and the provider continuation is permanently stranded until the session is reset. Consider calling `request.clearIfCurrent` (or `request.dispatchIfCurrent(Effect.void)`) in the `catchCause` handler before logging so the adapter's sticky flag is cleared on failure.

Comment on lines +188 to +193
const row = (yield* readRows()).find(
(candidate) =>
candidate.workspaceRoot === normalized &&
(options?.includeDeleted === true || candidate.deletedAt === null),
);
return row === undefined ? Option.none() : Option.some(yield* hydrateAvailable(row));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium project/ProjectService.ts:188

When a workspace root is reused by a new project after the original project was soft-deleted, getByWorkspaceRoot with includeDeleted: true returns the older deleted project instead of the active one. This yields stale metadata and the wrong projectId for callers that expect the current project occupying that workspace.

The method searches listAll() (ordered by creation time) with find, so it always selects the first matching row — which is the earliest created project — regardless of whether a newer, non-deleted project now owns the workspace. Consider selecting the active project first, falling back to deleted entries only when no active match exists.

-    const row = (yield* readRows()).find(
-      (candidate) =>
-        candidate.workspaceRoot === normalized &&
-        (options?.includeDeleted === true || candidate.deletedAt === null),
-    );
-    return row === undefined ? Option.none() : Option.some(yield* hydrateAvailable(row));
+    const rows = (yield* readRows()).filter(
+      (candidate) => candidate.workspaceRoot === normalized,
+    );
+    const row = rows.find((candidate) => candidate.deletedAt === null) ??
+      (options?.includeDeleted === true ? rows[0] : undefined);
+    return row === undefined ? Option.none() : Option.some(yield* hydrateAvailable(row));
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/project/ProjectService.ts around lines 188-193:

When a workspace root is reused by a new project after the original project was soft-deleted, `getByWorkspaceRoot` with `includeDeleted: true` returns the older deleted project instead of the active one. This yields stale metadata and the wrong `projectId` for callers that expect the current project occupying that workspace.

The method searches `listAll()` (ordered by creation time) with `find`, so it always selects the first matching row — which is the earliest created project — regardless of whether a newer, non-deleted project now owns the workspace. Consider selecting the active project first, falling back to deleted entries only when no active match exists.

if (event.appThread.id === input.threadId) {
return [true, state];
}
const isOwnedSubagent =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High orchestration-v2/RunExecutionService.ts:323

routeProviderEvent accepts app_thread.created for any subagent whose parent thread is owned, regardless of which run or attempt spawned it. During a steer (superseded attempt), both the old and new attempts subscribe to events on the same root thread, so a child thread created by one attempt is added to the other attempt's routing state. Subsequent node.updated, message.updated, turn_item.updated, and plan.updated events for that child are then routed to and ingested by both attempts, writing the child's data under each caller's runId and corrupting the other run's projections. The app_thread.created lineage carries no run or attempt discriminator, so the parent-thread check alone cannot distinguish ownership between concurrent attempts on the same root thread. Consider tracking the run/attempt that created each child thread (e.g. by refusing app_thread.created events that arrive before the owning attempt has observed its own provider turn) or documenting why cross-attempt sharing of child threads is intentional.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/RunExecutionService.ts around line 323:

`routeProviderEvent` accepts `app_thread.created` for any subagent whose parent thread is owned, regardless of which run or attempt spawned it. During a steer (superseded attempt), both the old and new attempts subscribe to events on the same root thread, so a child thread created by one attempt is added to the other attempt's routing state. Subsequent `node.updated`, `message.updated`, `turn_item.updated`, and `plan.updated` events for that child are then routed to and ingested by both attempts, writing the child's data under each caller's `runId` and corrupting the other run's projections. The `app_thread.created` lineage carries no run or attempt discriminator, so the parent-thread check alone cannot distinguish ownership between concurrent attempts on the same root thread. Consider tracking the run/attempt that created each child thread (e.g. by refusing `app_thread.created` events that arrive before the owning attempt has observed its own provider turn) or documenting why cross-attempt sharing of child threads is intentional.

) {
throw new Error("Cursor recordings cannot use both interrupt triggers.");
}
if (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low Adapters/CursorAdapterV2.testkit.ts:742

restartBeforePromptIndex is never range-validated. A negative value or an index >= prompts.length silently matches nothing in the prompt loop, so the recording completes without performing the requested agent close/resume sequence and produces a fixture that does not test the resume path. The other two interrupt fields validate their bounds; this one is missing the same check. Consider adding a guard rejecting values outside 0..prompts.length (inclusive of prompts.length if a post-loop restart is intended).

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Adapters/CursorAdapterV2.testkit.ts around line 742:

`restartBeforePromptIndex` is never range-validated. A negative value or an index `>= prompts.length` silently matches nothing in the prompt loop, so the recording completes without performing the requested agent close/resume sequence and produces a fixture that does not test the resume path. The other two interrupt fields validate their bounds; this one is missing the same check. Consider adding a guard rejecting values outside `0..prompts.length` (inclusive of `prompts.length` if a post-loop restart is intended).

juliusmarminge and others added 4 commits July 24, 2026 16:43
Update every deterministic Codex turn/start expectation with the explicit user approvals reviewer emitted by the adapter, restoring exact replay coverage and unblocking the merge lane.
…ojects (#3640)

Restore versioned shell-cache hydration, reconcile repository enrichment across multiple environments, and decode non-null settled shell timestamps correctly.
Normalize subagent result disclosure across timeline rendering and collapsed/expanded states.

Co-authored-by: codex <codex@users.noreply.github.com>
Expose MCP tools for worktree handoff and status inspection, with focused server and contract coverage.

Co-authored-by: codex <codex@users.noreply.github.com>
pendingRequest === null
? null
: { id: pendingRequest.id, kind: pendingRequest.kind, createdAt: pendingRequest.createdAt },
latestVisibleMessage: null,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low state/use-thread-selection.ts:95

threadDetailToShell always sets latestVisibleMessage to null, so when this fallback shell is used (e.g. for a newly created thread), consumers see an empty message preview even after messages exist in projection.messages. The projection already contains enough data to derive the latest visible message. Consider computing it from projection.messages instead of hardcoding null.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/state/use-thread-selection.ts around line 95:

`threadDetailToShell` always sets `latestVisibleMessage` to `null`, so when this fallback shell is used (e.g. for a newly created thread), consumers see an empty message preview even after messages exist in `projection.messages`. The projection already contains enough data to derive the latest visible message. Consider computing it from `projection.messages` instead of hardcoding `null`.

Comment thread apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.ts Outdated
}),
),
getThreadSnapshot: (threadId) =>
Effect.fail(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low orchestration-v2/Orchestrator.ts:5365

layerUnavailable.getShellSnapshot returns an OrchestratorProjectionError with a fabricated ThreadId.make("thread:shell"), so the error message references a nonexistent thread ID (thread:shell) instead of accurately reporting that the runtime is unavailable. Shell snapshot failures are not thread-specific, so consumers and logs see a misleading thread identifier. Consider using an error type that does not require a threadId, or document why a synthetic ID is used here.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Orchestrator.ts around line 5365:

`layerUnavailable.getShellSnapshot` returns an `OrchestratorProjectionError` with a fabricated `ThreadId.make("thread:shell")`, so the error message references a nonexistent thread ID (`thread:shell`) instead of accurately reporting that the runtime is unavailable. Shell snapshot failures are not thread-specific, so consumers and logs see a misleading thread identifier. Consider using an error type that does not require a `threadId`, or document why a synthetic ID is used here.

...(typeof record.tag === "string" ? { method: structuralProtocolMethod(record.tag) } : {}),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High provider/NativeProtocolLogging.ts:35

summarizeNativeProtocolPayload copies the raw tag field from untrusted provider payloads into the observability stream via structuralProtocolMethod(record.tag). Any payload whose tag value matches ^[A-Za-z][A-Za-z0-9._:/-]*$ and is ≤ 128 chars — e.g. an alphanumeric API key — passes the regex unchanged and is logged verbatim, contradicting the function's stated guarantee that payload values are never copied. The regex only validates format, not provenance, so provider-controlled values like credentials or prompt fragments slip through. Consider replacing structuralProtocolMethod(record.tag) with a fixed, known-safe tag (or a truncated/redacted form) so untrusted content is never emitted.

-      ...(typeof record.tag === "string" ? { method: structuralProtocolMethod(record.tag) } : {}),
+      ...(typeof record.tag === "string" ? { method: "tag" } : {}),
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/provider/NativeProtocolLogging.ts around line 35:

`summarizeNativeProtocolPayload` copies the raw `tag` field from untrusted provider payloads into the observability stream via `structuralProtocolMethod(record.tag)`. Any payload whose `tag` value matches `^[A-Za-z][A-Za-z0-9._:/-]*$` and is ≤ 128 chars — e.g. an alphanumeric API key — passes the regex unchanged and is logged verbatim, contradicting the function's stated guarantee that payload values are never copied. The regex only validates format, not provenance, so provider-controlled values like credentials or prompt fragments slip through. Consider replacing `structuralProtocolMethod(record.tag)` with a fixed, known-safe tag (or a truncated/redacted form) so untrusted content is never emitted.

Comment on lines +48 to +52
const replayDir = yield* fileSystem
.makeTempDirectory({
prefix: `t3-orchestration-v2-acp-registry-replay-${transcript.scenario}-`,
})
.pipe(Effect.orDie);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low Adapters/AcpRegistryAdapterV2.testkit.ts:48

transcript.scenario is interpolated directly into the makeTempDirectory prefix without sanitization, unlike makeReplayServerConfig which sanitizes the same value with scenario.replace(/[^a-z0-9_-]+/gi, "-"). A transcript with a scenario containing / or \ makes the prefix contain path separators, so makeTempDirectory targets a nested path that doesn't exist (or an unintended path), causing the ACP Registry replay to fail before starting. Consider applying the same sanitization used in makeReplayServerConfig.

Suggested change
const replayDir = yield* fileSystem
.makeTempDirectory({
prefix: `t3-orchestration-v2-acp-registry-replay-${transcript.scenario}-`,
})
.pipe(Effect.orDie);
const replayDir = yield* fileSystem
.makeTempDirectory({
prefix: `t3-orchestration-v2-acp-registry-replay-${transcript.scenario.replace(/[^a-z0-9_-]+/gi, "-")}-`,
})
.pipe(Effect.orDie);
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Adapters/AcpRegistryAdapterV2.testkit.ts around lines 48-52:

`transcript.scenario` is interpolated directly into the `makeTempDirectory` prefix without sanitization, unlike `makeReplayServerConfig` which sanitizes the same value with `scenario.replace(/[^a-z0-9_-]+/gi, "-")`. A transcript with a scenario containing `/` or `\` makes the prefix contain path separators, so `makeTempDirectory` targets a nested path that doesn't exist (or an unintended path), causing the ACP Registry replay to fail before starting. Consider applying the same sanitization used in `makeReplayServerConfig`.

interruptAndAwaitTerminal: (input) =>
Effect.gen(function* () {
const loaded = yield* load({ ...input, operation: "restart" });
if (Option.isNone(loaded.session)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium orchestration-v2/ProviderTurnControlService.ts:189

interruptAndAwaitTerminal returns immediately whenever load supplies Option.none() for the session, even when the interrupted attempt is still projected as running. This bypasses the loop that waits for both the provider turn and the attempt to terminalize, so the worker can detach or restart the provider session while the attempt projection is still in a running state. The early-return only checks providerTurn.status === "running"; when the turn has already terminalized but the attempt hasn't, the method returns without waiting for attempt finalization.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/ProviderTurnControlService.ts around line 189:

`interruptAndAwaitTerminal` returns immediately whenever `load` supplies `Option.none()` for the session, even when the interrupted attempt is still projected as `running`. This bypasses the loop that waits for both the provider turn and the attempt to terminalize, so the worker can detach or restart the provider session while the attempt projection is still in a `running` state. The early-return only checks `providerTurn.status === "running"`; when the turn has already terminalized but the attempt hasn't, the method returns without waiting for attempt finalization.

childRunId: childRun?.id ?? null,
childNodeId: task.id,
status,
providerInstanceId: ProviderInstanceId.make(task.driver),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium mcp/OrchestratorMcpService.ts:797

readTask returns ProviderInstanceId.make(task.driver) for the providerInstanceId field, but the actual provider instance is stored in task.providerInstanceId. The response reports the driver kind (e.g. "openai") instead of the concrete instance identifier, so callers get the wrong value whenever a project has multiple instances of the same driver. Use task.providerInstanceId directly instead of branding task.driver.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/OrchestratorMcpService.ts around line 797:

`readTask` returns `ProviderInstanceId.make(task.driver)` for the `providerInstanceId` field, but the actual provider instance is stored in `task.providerInstanceId`. The response reports the driver kind (e.g. `"openai"`) instead of the concrete instance identifier, so callers get the wrong value whenever a project has multiple instances of the same driver. Use `task.providerInstanceId` directly instead of branding `task.driver`.

Comment on lines +41 to +45
const replayDir = yield* fileSystem
.makeTempDirectory({
prefix: `t3-orchestration-v2-grok-replay-${transcript.scenario}-`,
})
.pipe(Effect.orDie);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low Adapters/GrokAdapterV2.testkit.ts:41

transcript.scenario is used unsanitized in the makeTempDirectory prefix, so a scenario containing / or \ causes makeTempDirectory to treat it as nested path components. The directory creation fails or writes outside the intended temp-prefix location, preventing the Grok replay from running. Compare with makeReplayServerConfig, which sanitizes the scenario with scenario.replace(/[^a-z0-9_-]+/gi, "-") before using it in a prefix. Consider applying the same sanitization here.

      const replayDir = yield* fileSystem
        .makeTempDirectory({
-          prefix: `t3-orchestration-v2-grok-replay-${transcript.scenario}-`,
+          prefix: `t3-orchestration-v2-grok-replay-${transcript.scenario.replace(/[^a-z0-9_-]+/gi, "-")}-`,
        })
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Adapters/GrokAdapterV2.testkit.ts around lines 41-45:

`transcript.scenario` is used unsanitized in the `makeTempDirectory` prefix, so a scenario containing `/` or `\` causes `makeTempDirectory` to treat it as nested path components. The directory creation fails or writes outside the intended temp-prefix location, preventing the Grok replay from running. Compare with `makeReplayServerConfig`, which sanitizes the scenario with `scenario.replace(/[^a-z0-9_-]+/gi, "-")` before using it in a prefix. Consider applying the same sanitization here.

export function makeClaudeAgentSdkReplayLayer(
transcript: ClaudeAgentSdkReplayTranscript,
): Layer.Layer<ClaudeAgentSdkQueryRunner> {
const queryRunner = makeReplayQueryRunner(transcript);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/ClaudeAdapterV2.testkit.ts:714

makeClaudeAgentSdkReplayLayer constructs makeReplayQueryRunner(transcript) outside the Layer.effect acquisition, so a single mutable replay runner is shared across every build of the layer. After one build consumes the transcript entries, a subsequent build of the same layer starts with an already-advanced cursor and immediately fails as exhausted/incomplete instead of replaying from the beginning. Construct makeReplayQueryRunner(transcript) inside the Effect.gen block so each build gets a fresh runner.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.testkit.ts around line 714:

`makeClaudeAgentSdkReplayLayer` constructs `makeReplayQueryRunner(transcript)` outside the `Layer.effect` acquisition, so a single mutable replay runner is shared across every build of the layer. After one build consumes the transcript entries, a subsequent build of the same layer starts with an already-advanced cursor and immediately fails as exhausted/incomplete instead of replaying from the beginning. Construct `makeReplayQueryRunner(transcript)` inside the `Effect.gen` block so each build gets a fresh runner.

const resumedRuntime = query({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/ClaudeAdapterV2.testkit.ts:1593

When the resumed query iterator throws after resumedRuntime is created at line 1593, the catch block only closes sourceRuntime and sourcePromptQueueresumedRuntime is never closed, leaking the Claude subprocess and its resources. The catch handler does not have access to resumedRuntime because it is declared inside the try block. Consider hoisting resumedRuntime above the try (or adding a nested finally around the resumed-query section) so it is closed on the error path.

-    const resumedRuntime = query({
+    let resumedRuntime: ReturnType<typeof query> | null = null;
+    resumedRuntime = query({
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.testkit.ts around line 1593:

When the resumed query iterator throws after `resumedRuntime` is created at line 1593, the `catch` block only closes `sourceRuntime` and `sourcePromptQueue` — `resumedRuntime` is never closed, leaking the Claude subprocess and its resources. The `catch` handler does not have access to `resumedRuntime` because it is declared inside the `try` block. Consider hoisting `resumedRuntime` above the `try` (or adding a nested `finally` around the resumed-query section) so it is closed on the error path.

resumedPromptQueue.offer(resumedMessage);
resumedPromptQueue.close();
const resumedIterator = resumedRuntime[Symbol.asyncIterator]();
await recordMessagesUntilIteratorDone({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High Adapters/ClaudeAdapterV2.testkit.ts:1600

The resumed half of recordClaudeResumeAtCursorQuery calls recordMessagesUntilIteratorDone (line 1600), which only returns when the SDK iterator reports done. A completed Claude turn emits a terminal result message but the iterator stays open until explicitly closed, so the recording blocks indefinitely and never reaches resumedRuntime.close() on line 1605. Use recordMessagesUntilTurnResultWithCursor (or equivalent) to stop after the result message, then close the runtime.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.testkit.ts around line 1600:

The resumed half of `recordClaudeResumeAtCursorQuery` calls `recordMessagesUntilIteratorDone` (line 1600), which only returns when the SDK iterator reports `done`. A completed Claude turn emits a terminal `result` message but the iterator stays open until explicitly closed, so the recording blocks indefinitely and never reaches `resumedRuntime.close()` on line 1605. Use `recordMessagesUntilTurnResultWithCursor` (or equivalent) to stop after the `result` message, then close the runtime.

frame: makeClaudePromptOfferFrame(message),
});

try {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/ClaudeAdapterV2.testkit.ts:1429

When a restarted query throws during iteration, the SDK query runtime is never closed. queryRuntime is declared inside the try block, so the catch can only close promptQueue — the underlying SDK process/resources remain alive after the error is rethrown. Move the queryRuntime declaration above the try and close it in a finally block (or in the catch) so the runtime is cleaned up on error.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.testkit.ts around line 1429:

When a restarted query throws during iteration, the SDK query runtime is never closed. `queryRuntime` is declared inside the `try` block, so the `catch` can only close `promptQueue` — the underlying SDK process/resources remain alive after the error is rethrown. Move the `queryRuntime` declaration above the `try` and close it in a `finally` block (or in the `catch`) so the runtime is cleaned up on error.

if (tasksQuery.error === null && boundTasks.length === 0) return null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low chat/ThreadAutomationsPanel.tsx:59

The panel disappears during initial load even when the thread has bound automations. While scheduledTasksLive is still loading, tasksQuery.data is null, tasksQuery.error is null, and boundTasks is empty, so the early return at line 59 fires — hiding the entire section until the subscription resolves. Add !tasksQuery.isPending to the hide condition (or render a loading state) so the panel doesn't incorrectly show absence on every initial load.

Suggested change
if (tasksQuery.error === null && boundTasks.length === 0) return null;
if (!tasksQuery.isPending && tasksQuery.error === null && boundTasks.length === 0) return null;
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/ThreadAutomationsPanel.tsx around line 59:

The panel disappears during initial load even when the thread has bound automations. While `scheduledTasksLive` is still loading, `tasksQuery.data` is `null`, `tasksQuery.error` is `null`, and `boundTasks` is empty, so the early return at line 59 fires — hiding the entire section until the subscription resolves. Add `!tasksQuery.isPending` to the hide condition (or render a loading state) so the panel doesn't incorrectly show absence on every initial load.

Resolve migration sequencing and carry snooze behavior through orchestration V2.

Co-authored-by: codex <codex@users.noreply.github.com>
@juliusmarminge
juliusmarminge marked this pull request as ready for review July 24, 2026 15:31
const userDataDirName = isDevelopment ? "t3code-dev" : "t3code";
const legacyUserDataDirName = isDevelopment ? "T3 Code (Dev)" : "T3 Code (Alpha)";
const userDataDirName = isDevelopment ? "t3code-dev" : "t3code-v2";
const legacyUserDataDirName = isDevelopment ? "T3 Code (Dev)" : "T3 Code (Alpha v2)";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Desktop legacy userdata not found

High Severity

Upgrading users lose their settings, connections, and catalogs. The app now probes for user data in new, versioned directories (userdata-v2, t3code-v2, T3 Code (Alpha v2)) while existing installs use unversioned paths. This creates a fresh profile, despite the branding remaining T3 Code (Alpha).

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c841e2a. Configure here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the intentional stack boundary addressed by follow-up #4400. Its rebased head restores the existing unversioned desktop/server profile paths and adds migration 43 to import v1 state in place; the focused migration/import suite and a clean 1→43 startup smoke pass. #4400 is based on the exact #2829 head and will be rebased again after the remaining bounded parent fixes.

navigation.navigate("Thread", {
environmentId: props.environmentId,
threadId: targetThreadId,
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fork opens before shell ready

Medium Severity

After a successful fork command, navigation to the new thread runs even when waitForThreadShell times out without a shell row. The web client waits for the shell and blocks navigation with an error when it never arrives.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c841e2a. Configure here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in bccaea5. The mobile wait now returns readiness, blocks navigation on timeout, and shows a recovery message. Added focused ready/timeout regression coverage; mobile typecheck/lint/format passed and a real iOS Codex session completed on the updated bundle.

Co-authored-by: codex <codex@users.noreply.github.com>
Comment thread apps/server/src/orchestration-v2/ThreadForkService.ts
);
}

await recordInterruptedClaudeQuery({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/ClaudeAdapterV2.testkit.ts:2074

When interruptAfter is "tool_use" and the first prompt completes without emitting a tool-use message, recordInterruptedClaudeQuery throws from recordMessagesUntilFirstToolUse before reaching its cleanup block, leaving the first query runtime and prompt queue open and omitting the first runtime_exit entry. The exception propagates into recordClaudeInterruptRestartQuery, which aborts before starting the second (restart) query, so the replay transcript is incomplete and contains a leaked runtime. Consider wrapping the recordInterruptedClaudeQuery call in a try/catch (or ensuring the helper cleans up on early throw) so the restart recording can proceed.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.testkit.ts around line 2074:

When `interruptAfter` is `"tool_use"` and the first prompt completes without emitting a tool-use message, `recordInterruptedClaudeQuery` throws from `recordMessagesUntilFirstToolUse` before reaching its cleanup block, leaving the first query runtime and prompt queue open and omitting the first `runtime_exit` entry. The exception propagates into `recordClaudeInterruptRestartQuery`, which aborts before starting the second (restart) query, so the replay transcript is incomplete and contains a leaked runtime. Consider wrapping the `recordInterruptedClaudeQuery` call in a try/catch (or ensuring the helper cleans up on early throw) so the restart recording can proceed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 408efa6. The tool-use wait is inside the cleanup/runtime-exit boundary; the regression proves close, runtime_exit(error), and prompt queue completion when no tool use arrives.

Comment thread apps/server/scripts/acp-mock-agent.ts
input.row.item.type === "user_message" || input.row.item.type === "assistant_message"
? input.row.item.messageId
: null;
const message =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low mcp/OrchestratorMcpService.ts:566

In forked threads, inherited user/assistant timeline items are returned with createdBy and creationSource set to null instead of their actual provenance. The lookup at line 569 searches only input.projection.messages, which contains the target thread's own messages — but inherited rows have sourceThreadId pointing to a different thread, so their messages are never found and the ?? null fallback silently drops provenance. Consider resolving message provenance from the source thread when row.sourceThreadId differs from the target thread, or including inherited messages in the projection's messages array.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/OrchestratorMcpService.ts around line 566:

In forked threads, inherited user/assistant timeline items are returned with `createdBy` and `creationSource` set to `null` instead of their actual provenance. The lookup at line 569 searches only `input.projection.messages`, which contains the target thread's own messages — but inherited rows have `sourceThreadId` pointing to a different thread, so their messages are never found and the `?? null` fallback silently drops provenance. Consider resolving message provenance from the source thread when `row.sourceThreadId` differs from the target thread, or including inherited messages in the projection's `messages` array.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b244181. Thread reads load only distinct inherited message-source projections on the requested page, project-scoped, and resolve provenance by sourceThreadId. The integration regression verifies inherited agent/MCP provenance from a real fork.

let cursorAdvanced = makeCursorSignal();
let activeOptions: ClaudeAgentSdkQueryOptions | null = null;

const fail = (error: ClaudeAgentSdkReplayError): never => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High Adapters/ClaudeAdapterV2.testkit.ts:424

When assertNextOutboundFrame detects a frame mismatch, it throws a replay error without resolving cursorAdvanced. If replayMessages is concurrently awaiting cursorAdvanced.promise on the same expect_outbound entry, the generator never resumes and the message stream hangs forever — consumers waiting for stream completion or the error to surface are blocked indefinitely. The fail helper should resolve the cursor signal before throwing so the blocked await wakes up and the stream terminates.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.testkit.ts around line 424:

When `assertNextOutboundFrame` detects a frame mismatch, it throws a replay error without resolving `cursorAdvanced`. If `replayMessages` is concurrently awaiting `cursorAdvanced.promise` on the same `expect_outbound` entry, the generator never resumes and the message stream hangs forever — consumers waiting for stream completion or the error to surface are blocked indefinitely. The `fail` helper should resolve the cursor signal before throwing so the blocked `await` wakes up and the stream terminates.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 408efa6. Replay failure now resolves cursorAdvanced before throwing; the focused regression proves a concurrently waiting stream terminates on outbound mismatch.

Comment thread apps/server/src/orchestration-v2/SubagentProjection.ts
juliusmarminge and others added 3 commits July 24, 2026 17:54
Reset snooze metadata when creating forks and provider subagent children, with focused regressions for both inheritance paths.

Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Wake replay stream waiters when outbound validation fails and ensure interrupted recordings close when no tool use arrives.

Co-authored-by: codex <codex@users.noreply.github.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is ON, but a cloud agent failed to start.

Reviewed by Cursor Bugbot for commit 408efa6. Configure here.

sourceThreadIds,
(sourceThreadId) => loadProjectThread(target.thread.projectId, sourceThreadId),
{ concurrency: 8 },
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MCP read fails deleted source

Medium Severity

Reading a forked thread now fails with thread_not_found if its timeline includes messages inherited from a deleted source. The t3_thread_read operation attempts to load the deleted source's projection for message metadata, causing the entire read to error.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 408efa6. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL 1,000+ changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants